Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed GetNetworkInterfaces method to to retrieve information related to all interfaces #100824

Merged
merged 8 commits into from
Apr 20, 2024

Conversation

MojtabaTajik
Copy link
Contributor

Based on Microsoft documentation, we should pass "GAA_FLAG_INCLUDE_ALL_INTERFACES" to retrieve information related to all interfaces.

Fix #89990

…per speeds; non-positive speeds indicates absence of physical address.
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test failure seems related

  Starting:    System.Net.NetworkInformation.Functional.Tests (parallel test collections = on [2 threads], stop on fail = off)
    System.Net.NetworkInformation.Tests.NetworkInterfaceBasicTest.BasicTest_AccessInstanceProperties_NoExceptions [FAIL]
      Assert.Equal() Failure: Values differ
      Expected: 6
      Actual:   0
      Stack Trace:
        /_/src/libraries/System.Net.NetworkInformation/tests/FunctionalTests/NetworkInterfaceBasicTest.cs(55,0): at System.Net.NetworkInformation.Tests.NetworkInterfaceBasicTest.BasicTest_AccessInstanceProperties_NoExceptions()
           at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
        /_/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.CoreCLR.cs(36,0): at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
        /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs(57,0): at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
      Output:
        - NetworkInterface -
        Name: Ethernet 2
        Description: Microsoft Hyper-V Network Adapter #2
        ID: {5A812509-8FA5-4802-9EAC-92A43D5C3D7D}
        IsReceiveOnly: False
        Type: Ethernet
        Status: Up
        Speed: 50000000000
        SupportsMulticast: True
        GetPhysicalAddress(): 000D3A5A03C9
        - NetworkInterface -
        Name: Loopback Pseudo-Interface 1
        Description: Software Loopback Interface 1
        ID: {46BCBBDA-D902-11EE-A1F4-806E6F6E6963}
        IsReceiveOnly: False
        Type: Loopback
        Status: Up
        Speed: 1073741824
        SupportsMulticast: True
        GetPhysicalAddress(): 
        - NetworkInterface -
        Name: vEthernet (nat)
        Description: Hyper-V Virtual Ethernet Adapter
        ID: {30CAB5C1-869C-4879-AF7F-BF5232024FC8}
        IsReceiveOnly: False
        Type: Ethernet
        Status: Up
        Speed: 10000000000
        SupportsMulticast: True
        GetPhysicalAddress(): 00155D4F8F8B
        - NetworkInterface -
        Name: vSwitch (nat)-Hyper-V Virtual Switch Extension Filter-0000
        Description: Hyper-V Virtual Switch Extension Adapter-Hyper-V Virtual Switch Extension Filter-0000
        ID: {9330C02E-F697-11EE-A203-000D3A5A03C9}
        IsReceiveOnly: False
        Type: Ethernet
        Status: Up
        Speed: 10000000000
        SupportsMulticast: True
        GetPhysicalAddress(): 
  Finished:    System.Net.NetworkInformation.Functional.Tests
=== TEST EXECUTION SUMMARY ===
   System.Net.NetworkInformation.Functional.Tests  Total: 244, Errors: 0, Failed: 1, Skipped: 0, Time: 0.725s

@MojtabaTajik
Copy link
Contributor Author

@wfurt I just investigated the issues related to the change because now we list all interfaces.
The failed test is related to this line in tests:

if (nic.NetworkInterfaceType == NetworkInterfaceType.Ethernet && nic.Speed > 0)
{
Assert.Equal(6, nic.GetPhysicalAddress().GetAddressBytes().Length);
}

Based on test logs, it fails on "Microsoft Hyper-V Network Adapter" I just enabled Hyper-V on my local and got the same error. After investigation, I figured out there are some NICs without a Mac address, so this assertion part of this test is invalid and should be removed.

Based on the documentation of GetAdaptersAddresses, this change will retrieve even those addresses associated with adapters not bound to an address family specified in the Family parameter.

If the GAA_FLAG_INCLUDE_ALL_INTERFACES is set, then all NDIS adapters will be retrieved even those addresses associated with adapters not bound to an address family specified in the Family parameter. When this flag is not set, then only the addresses that are bound to an adapter enabled for the address family specified in the Family parameter are returned.

I just removed that assertion because it's not valid anymore.

@wfurt
Copy link
Member

wfurt commented Apr 17, 2024

I just removed that assertion because it's not valid anymore.

what does it return @MojtabaTajik? I'm generally ok with the approach. However it seems like this only one call to GetPhysicalAddress().GetAddressBytes() so it may be in just to see it does not crash or throw. If the length is predictable perhaps we can something like Assert.True(length == 0 || length == 6) ??? If that works could we perhaps even drop the check for Speed???

@MojtabaTajik
Copy link
Contributor Author

MojtabaTajik commented Apr 18, 2024

@wfurt Indeed, it returns 0 for those NICs that lack a physical address.
I just updated my PR.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @MojtabaTajik for the contribution.

@wfurt
Copy link
Member

wfurt commented Apr 20, 2024

all test failures are known errors.

@wfurt wfurt merged commit 76293f8 into dotnet:main Apr 20, 2024
84 of 87 checks passed
@LoopedBard3
Copy link
Member

Related performance regressions:
Windows x64: dotnet/perf-autofiling-issues#33051

matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
…d to all interfaces (dotnet#100824)

* Add IncludeAllInterfaces as falg to reterive all avaliable interfaces including disabled ones

* Update tests to accommodate new adapters. Ensure adapters reflect proper speeds; non-positive speeds indicates absence of physical address.

* Removed unnecessary assertion

* Add assertion over physical address bytes
@karelz karelz added this to the 9.0.0 milestone May 14, 2024
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
…d to all interfaces (dotnet#100824)

* Add IncludeAllInterfaces as falg to reterive all avaliable interfaces including disabled ones

* Update tests to accommodate new adapters. Ensure adapters reflect proper speeds; non-positive speeds indicates absence of physical address.

* Removed unnecessary assertion

* Add assertion over physical address bytes
@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NetworkInterface.GetAllNetworkInterfaces() doesn't list disabled adapters on Windows
4 participants